-
-
Notifications
You must be signed in to change notification settings - Fork 110
QR codes and symmetric encryption for broadcast channels #7268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| } else if chat.typ == Chattype::InBroadcast && contact_id == ContactId::SELF { | ||
| // For incoming broadcast channels, it's not possible to remove members, | ||
| // but it's possible to leave: | ||
| let self_addr = context.get_primary_self_addr().await?; | ||
| send_member_removal_msg(context, &chat, contact_id, &self_addr).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a broadcast channel now uses the same code as leaving a group, so, we don't need this block anymore.
See this code a few lines above:
if matches!(
chat.typ,
Chattype::Group | Chattype::OutBroadcast | Chattype::InBroadcast
) {| if is_contact_in_chat(context, chat_id, contact_id).await? { | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this block because we're in the else clause of if is_contact_in_chat(context, chat_id, contact_id).await?, so, this was dead code
| "invalid contact_id {} for adding to group", | ||
| contact_id | ||
| ); | ||
| ensure!(!chat.is_mailing_list(), "Mailing lists can't be changed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this line because above we're ckecking ensure!(chat.typ == Chattype::Group || chat.typ == Chattype::OutBroadcast [...]), so, this was dead code
| hash_alg: HashAlgorithm::default(), | ||
| salt, | ||
| }; | ||
| let mut msg = msg.seipd_v2( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses SEIPD v2, while in other places, we use SEIPD v1. I assume that we used SEIPD v1 because v2 didn't exist yet at the time, and SEIPD v2 is superior, so, we should use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no tests break, i guess that we should, but this can be done in another PR. Maybe check compatibility with Thunderbird also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are encrypting symmetrically, so Thunderbird won't be able to decrypt it, anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, if we want to switch pk_encrypt() to SEIPDv2, it may make sense to check compatibility with TB. For this PR it's not necessary of course
Another idea to speed it up would be to track/remember which secrets were used most recently and then try them in the order of when they were last used, so if the recipient has lots of old broadcast channels then those won't slow down decryption in the more active broadcast channels. |
Co-authored-by: iequidoo <[email protected]>
Co-authored-by: iequidoo <[email protected]>
…ssign the message to 1:1 chat with the sender instead
…failing Otherwise, if Bob joins a channel and later leaves it, and then Alice turns on her second device, Alice's second device will see the request-with-auth message and re-add Bob.
Co-authored-by: iequidoo <[email protected]>
…f _verified) again
|
I just noticed another problem: In a multi-device setup where Alice1 uses DC v2.22.0 and Alice2 uses this PR, and Alice2 performs a securejoin to add someone to a channel, Alice1 will be stuck in an infinite loop similar to #7290 because The solution is probably to use a header different than List-Id, so that old versions of DC don't recognize it as a mailing list. |
Off-topic, but we're going to have more bugs like this if we don't fix this logic in the IMAP loop. In #7321 i suggested a way to simplify |
Without this commit, there is this problem: In a multi-device setup where Alice1 uses DC v2.22.0 and Alice2 uses this PR, and Alice2 performs a securejoin to add someone to a channel, Alice1 will be stuck in an infinite loop similar to #7290 because `observe_securejoin_on_other_device()` fails. The solution is to use a header different than List-Id, so that old versions of DC don't recognize it as a mailing list.
Follow-up for #7042, part of #6884.
This will make it possible to create invite-QR codes for broadcast channels, and make them symmetrically end-to-end encrypted.
Backwards compatibility
This is not very backwards compatible:
The symmetric encryption
Symmetric encryption uses a shared secret. Currently, we use AES128 for encryption everywhere in Delta Chat, so, this is what I'm using for broadcast channels (though it wouldn't be hard to switch to AES256).
The secret shared between all members of a broadcast channel has 258 bits of entropy (see
fn create_broadcast_shared_secretin the code).Since the shared secrets have more entropy than the AES session keys, it's not necessary to have a hard-to-compute string2key algorithm, so, I'm using the string2key algorithm
salted. This is fast enough that Delta Chat can just try out all known shared secrets. 1 In order to prevent DOS attacks, Delta Chat will not attempt to decrypt with a string2key algorithm other thansalted2.The "Securejoin" protocol that adds members to the channel after they scanned a QR code
This PR uses the classical securejoin protocol, the same that is also used for group and 1:1 invitations.
The messages sent back and forth are called
vg-request,vg-auth-required,vg-request-with-auth, andvg-member-added. I considered using thevc-prefix, because from a protocol-POV, the distinction betweenvc-andvg-isn't important (as @link2xt pointed out in an in-person discussion), butvg-while broadcasts and 1:1 chats usedvc-,vc-member-addedmessage yet, so, this would mean one more different kind of messagevc-.Footnotes
In a symmetrically encrypted message, it's not visible which secret was used to encrypt without trying out all secrets. If this does turn out to be too slow in the future, then we can remember which secret was used more recently, and and try the most recent secret first. If this is still too slow, then we can assign a short, non-unique (~2 characters) id to every shared secret, and send it in cleartext. The receiving Delta Chat will then only try out shared secrets with this id. Of course, this would leak a little bit of metadata in cleartext, so, I would like to avoid it. ↩
A DOS attacker could send a message with a lot of encrypted session keys, all of which use a very hard-to-compute string2key algorithm. Delta Chat would then try to decrypt all of the encrypted session keys with all of the known shared secrets. In order to prevent this, as I said, Delta Chat will not attempt to decrypt with a string2key algorithm other than
salted↩